feat: add maximum submissions limit for forms#3199
feat: add maximum submissions limit for forms#3199LAfricain wants to merge 1 commit intonextcloud:mainfrom
Conversation
Chartman123
left a comment
Chartman123
left a comment
There was a problem hiding this comment.
@LAfricain thanks for your PR. I just had a quick look at your code and didn't test it yet in my instance. Already a few comments. :)
Chartman123
added
enhancement
Chartman123
left a comment
Chartman123
left a comment
There was a problem hiding this comment.
Some more comments :) Please have a look at the failing workflows, too.
|
Hello @Chartman123, |
|
@LAfricain sorry, I don't have much spare time at the moment :) Please fix the remaining workflow issues |
It's Ok, no rush. I didn't realize I had to fix the errors myself; I still need to get used to the tool. It's done now. |
|
@LAfricain yes, you have to fix them yourself (or use the npm scripts and php scripts). Environments like VSCode also can assist you with that :) |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
|
@LAfricain it's getting better :) only the DCO (sign-off your commits) and OpenAPI missing... Could you then please also squash all your commits into a single one? |
LAfricain
force-pushed
the
maxsubmissions
branch
from
4e655a8 to
0f256fc
Compare
|
Oh, please don't merge the main branch into this PR, always use |
LAfricain
force-pushed
the
maxsubmissions
branch
2 times, most recently
from
d1c6101 to
05bfd72
Compare
Chartman123
force-pushed
the
maxsubmissions
branch
from
05bfd72 to
06294e7
Compare
Chartman123
left a comment
Chartman123
left a comment
There was a problem hiding this comment.
a few more comments that came up during test
| // Check if max submissions limit is reached | ||
| $maxSubmissions = $form->getMaxSubmissions(); | ||
| if ($maxSubmissions !== null && $this->submissionMapper->countSubmissions($form->getId()) >= $maxSubmissions) { | ||
| return false; | ||
| } | ||
|
|
There was a problem hiding this comment.
This check fails once the submission reaches the limit as this part of the code is called after inserting the submission.
@susnux do you think a single check for maxSubmissions is enough in ApiController or do we need the double check here as well? If so, we need to change the condition from >= to >
LAfricain
force-pushed
the
maxsubmissions
branch
from
043a49d to
4246abe
Compare
|
@LAfricain please add maxSubmissions to the FormsMigrator.php import. And please don't merge the main branch into your branch. This adds lots of changed files to the PR that aren't related. Please use git rebase as I already wrote in my last comment :) |
Add the ability to limit the number of responses a form can receive. When the limit is reached, the form is automatically closed and displays a dedicated message instead of accepting new submissions. - Add max_submissions column to forms_v2_forms table (migration) - Add maxSubmissions property to Form entity - Check submission limit in FormsService::canSubmit() - Add limit enforcement in ApiController::newSubmission() - Add isMaxSubmissionsReached flag in form API response - Update FormsForm psalm type in ResponseDefinitions - Add limit settings UI in SettingsSidebarTab - Display dedicated 'Form is full' message in Submit view - Update openapi.json - Update unit and integration tests Closes nextcloud#596 Signed-off-by: lafricain79 <lafricain79@gmail.com>
LAfricain
force-pushed
the
maxsubmissions
branch
from
4246abe to
2e93e84
Compare
|
Hi @Chartman123, sorry for the mess with the main branch merge. I've cleaned everything up and the PR should now only contains the relevant files. I also added maxSubmissions to FormsMigrator.php as requested. Hope it's good now! 🙏 |
2 participants
Add the ability to limit the number of responses a form can receive. When the limit is reached, the form is automatically closed and displays a dedicated message instead of accepting new submissions.
Closes #596